Skip to content
This repository has been archived by the owner on Mar 9, 2022. It is now read-only.

Update default discovery and refresh config values #191

Merged
merged 1 commit into from
Nov 17, 2017
Merged

Conversation

wennmo
Copy link
Member

@wennmo wennmo commented Nov 16, 2017

Mira has a very short interval for discovering and checking health on engines. Changed the interval to:

  • 10 seconds interval for discovering new engines
  • 30 seconds interval for querying health and metrics on engines

@FredrikFolkesson
Copy link
Contributor

Is this because mira is chatty? I'm just thinking around when in production if you'd rather not have the shorter values.

Copy link
Contributor

@janmiderback janmiderback left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a comment on the selected interval

const defaultEngineDiscoveryRefreshRate = 1000;
const defaultEngineHealthRefreshRate = 5000;
const defaultEngineDiscoveryRefreshRate = 10000;
const defaultEngineHealthRefreshRate = 30000;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this not a bit too infrequent? I cannot tell what is right but every 30 s introduces quite some delay until an unhealthy engine is detected.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Docker default healthcheck interval for Mira is 30 seconds so that is why I chose that value.

This value in Config.js is also used as the interval for Mira to fetch metrics from each engine. Atleast in my opinion 5 seconds is a bit greedy for both health and metrics, and if a shorter interval is needed it is still configurable using env variables.

Copy link
Contributor

@janmiderback janmiderback left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Copy link
Contributor

@peol peol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, reasonable intervals.

We may need additional logic to update individual engines at some point based on external information (like rabbitmq, APIs) which will give us lower refresh rates automagically anyway.

@wennmo wennmo merged commit 8c05a99 into master Nov 17, 2017
@wennmo wennmo deleted the default_values branch November 17, 2017 06:26
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants